Add ability to respond to a message reaction being added or remove #520

ArcticSnowman · 2024-10-23T18:05:44Z

Proposed change

I worked on a slack chat bot that was based on https://github.com/ArcticSnowman/go-slackbot which I added the ability to respond to emojis (reaction).

So this is my first attempt to add reaction support to this bot framework. Hope this helps with #45

Types of changes

What types of changes is this pull request introducing to flottbot? Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

You can fill this out after creating your PR. Put an x in the boxes that apply

I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

coveralls · 2024-10-24T02:18:50Z

Pull Request Test Coverage Report for Build 11555364501

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 0.0%

Totals
Change from base Build 10426782526:	0.0%
Covered Lines:	0
Relevant Lines:	0

💛 - Coveralls

wass3r

thanks for the contribution! with a couple of caveats it works as advertised (see comments).

some further tweaks that would be interesting to explore are (and this definitely doesn't have to be in this PR):

separate the action (added/removed) so you can target either
check the target thread to see whether we already replied and, at least by default, don't post again

wass3r · 2024-10-24T03:20:15Z

config-example/rules/reaction.yaml

@@ -0,0 +1,11 @@
+name: plus-one-reaction
+active: true
+reaction_match: ":+1:"


did this exact rule work for you?

asking because, 1) i can only make it work without the surrounding : and 2) only if the value doesn't contain +, like heavy_plus_sign works without problem.

the issue is that we send the the string to regex in the matcher where + has special meaning, and the :s aren't part of the reaction value returned by the Slack API - at least in my test workspace.

if you throw :+1: as instead of :hello: in the test you added you will notice that it fails or maybe you saw that too.

Suggested change

reaction_match: ":+1:"

reaction_match: "heavy_plus_sign"

i think to make +1 work we will need to make a few more tweaks and maybe bypass regex to do an exact match. we can do it in a follow-up PR.

I've updated the example and fixed some issue I found. I decided that we need to treat addition and removals of reactions are separate in Rule and Message model so action can be triggered for one or other or both.

remote/slack/helper.go

Update example Some refactoring in matcher.go

wass3r

works as advertised. thanks for the tweaks. couple of small things and we're good i think.

i noticed you used plural reactions_added vs reaction_added. was the thought to be able to support matching multiple reactions in one rule at some point? if so, i think i like that. some workspaces have multiple emojis that are similar, eg. :question: and :grey_question:. nothing that has to be fleshed out in this PR though. just curious.

thank you!

Makefile

core/matcher.go

ArcticSnowman

Should

	if rule.Hear == "" && rule.ReactionsAdded == "" && rule.ReactionsRemoved == "" {

not be better as

	if rule.Respond != "" {

wass3r · 2024-10-25T14:57:37Z

Should

	if rule.Hear == "" && rule.ReactionsAdded == "" && rule.ReactionsRemoved == "" {

not be better as

	if rule.Respond != "" {

makes a lot of sense, i like it! let's do that. if you don't mind changing the comment above it as well 🥺 thank you!

ArcticSnowman · 2024-10-25T15:40:01Z

@wass3r - I went with plurals as the use case in the other bot library I use, does take multiple emoji triggers.

For this framework, the user would need to craft a regexp string to match multiple.

For example:

(plus_one|plusone|plus1)

ArcticSnowman · 2024-10-25T15:53:01Z

PR for docs target/flottbot-docs#60

wass3r · 2024-10-25T18:20:21Z

core/matcher.go

+//
+//nolint:gocyclo // refactor


looks like this addition is making the build and validate steps fail

wass3r · 2024-10-25T18:30:25Z

for the failing test (https://github.com/target/flottbot/actions/runs/11521113211/job/32081108474?pr=520#step:5:252), it looks like the improvement you made exposed a flaw in a testcase. in this block

flottbot/core/matcher_test.go

Lines 647 to 653 in dc4cfeb

    
           testRuleNeedArg := models.Rule{} 
        
           testRuleNeedArg.AllowUsers = []string{"fooUser"} 
        
           testRuleNeedArg.Args = []string{"arg1", "arg2"} 
        
           testMessageNeedArg := new(models.Message) 
        
           needArgVars := make(map[string]string) 
        
           needArgVars["_user.name"] = "fooUser" 
        
           testMessageNeedArg.Vars = needArgVars

we will need to add testRuleNeedArg.Respond = "foo" (for example)

of Hear, Respond, ReactionsAdded, or ReactionsRremoved Update matcher_tests

ArcticSnowman · 2024-10-25T19:37:35Z

@wass3r - Yeah those unit tests need an explicit Hear/Respond added. I've clean up the test and added a check to the function that requires one of Hear, Respond, ReactionsAdded, or ReactionsRemoved. to have a value to be valid.

wass3r · 2024-10-25T21:15:54Z

i think we're one commit away.. linter is (harshly) complaining about this line

flottbot/core/matcher.go

Line 244 in 63542e7

. happy to merge after that tiny edit. thanks for all the work!

ArcticSnowman · 2024-10-28T13:48:43Z

Fixed to keep the linter happy...

On the topic of a rule checking if it's replied to the reaction already, the script/code handling the reaction would need to either

Be able to call the Slack (or other service) API to interrogate if the bot had responded
Keep some stateful data to log the TimeStamp of the message reacted to.

wass3r · 2024-10-28T15:32:33Z

yup, i was thinking about utilizing https://api.slack.com/methods/conversations.replies . trying to shy away from adding the complexity of state management for users/admins at this time :)

wass3r

thank you!

Steven Arnott added 2 commits October 23, 2024 13:51

Add reaction handling

6ed7ff3

Example

839a42d

ArcticSnowman requested review from delta592, wass3rw3rk, sjqnn and JordanSussman as code owners October 23, 2024 18:05

Some lint clean up

4e684ee

wass3r requested changes Oct 24, 2024

View reviewed changes

Split add and remove action to seeperate places

2e944ad

Update example Some refactoring in matcher.go

wass3r requested changes Oct 25, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

core/matcher.go Outdated Show resolved Hide resolved

core/matcher.go Outdated Show resolved Hide resolved

ArcticSnowman commented Oct 25, 2024

View reviewed changes

Resolve PR comments

49172c1

Switch logic to if "rule.Respond" type

422d826

ArcticSnowman mentioned this pull request Oct 25, 2024

docs: Add reactions target/flottbot-docs#60

Merged

wass3r reviewed Oct 25, 2024

View reviewed changes

Added check to isValidHitChatRule where a rule needs one

63542e7

of Hear, Respond, ReactionsAdded, or ReactionsRremoved Update matcher_tests

Keep the linter happy

727b67b

wass3r approved these changes Oct 28, 2024

View reviewed changes

wass3rw3rk approved these changes Oct 28, 2024

View reviewed changes

wass3rw3rk merged commit 3bd3031 into target:main Oct 28, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to respond to a message reaction being added or remove #520

Add ability to respond to a message reaction being added or remove #520

ArcticSnowman commented Oct 23, 2024 •

edited

Loading

coveralls commented Oct 24, 2024 •

edited

Loading

wass3r left a comment

wass3r Oct 24, 2024 •

edited

Loading

ArcticSnowman Oct 24, 2024

wass3r left a comment •

edited

Loading

ArcticSnowman left a comment

wass3r commented Oct 25, 2024 •

edited by wass3rw3rk

Loading

ArcticSnowman commented Oct 25, 2024

ArcticSnowman commented Oct 25, 2024

wass3r Oct 25, 2024

wass3r commented Oct 25, 2024

ArcticSnowman commented Oct 25, 2024

wass3r commented Oct 25, 2024

ArcticSnowman commented Oct 28, 2024

wass3r commented Oct 28, 2024

wass3r left a comment

		//
		//nolint:gocyclo // refactor

Add ability to respond to a message reaction being added or remove #520

Add ability to respond to a message reaction being added or remove #520

Conversation

ArcticSnowman commented Oct 23, 2024 • edited Loading

Proposed change

Types of changes

Checklist

coveralls commented Oct 24, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11555364501

Details

💛 - Coveralls

wass3r left a comment

Choose a reason for hiding this comment

wass3r Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

ArcticSnowman Oct 24, 2024

Choose a reason for hiding this comment

wass3r left a comment • edited Loading

Choose a reason for hiding this comment

ArcticSnowman left a comment

Choose a reason for hiding this comment

wass3r commented Oct 25, 2024 • edited by wass3rw3rk Loading

ArcticSnowman commented Oct 25, 2024

ArcticSnowman commented Oct 25, 2024

wass3r Oct 25, 2024

Choose a reason for hiding this comment

wass3r commented Oct 25, 2024

ArcticSnowman commented Oct 25, 2024

wass3r commented Oct 25, 2024

ArcticSnowman commented Oct 28, 2024

wass3r commented Oct 28, 2024

wass3r left a comment

Choose a reason for hiding this comment

ArcticSnowman commented Oct 23, 2024 •

edited

Loading

coveralls commented Oct 24, 2024 •

edited

Loading

wass3r Oct 24, 2024 •

edited

Loading

wass3r left a comment •

edited

Loading

wass3r commented Oct 25, 2024 •

edited by wass3rw3rk

Loading